[fix] ES5 Object.keys only accepts an object - #27089

All CLA requirements met.

Heya Jordan Harband (@ljharb)! Looks like our baselines might need to be updated - it's likely the symbol baselines are printing out a different type for uses of Object.keys. Try running gulp baseline-accept or jake baseline-accept and git add tests. You can think of these as snapshot tests.

Additionally, is there a reason why Object.keys needed to be added to es2015.core.d.ts?

Daniel Rosenwasser (@DanielRosenwasser) yes, because the spec for Object.keys changed in ES2015 to no longer throw on non-nullish primitives (numbers, strings, booleans, symbols) - so while a type of object is correct for ES5, it's not correct for ES6+.

Thanks, I'll update the tests shortly.

Tests updated.

Is there an associated bug for this?

Ryan Cavanaugh (@RyanCavanaugh) no, i didn't file one - it seemed like this would be faster. If you need me to file one that copy-pastes the first two lines in the OP, i can certainly do that.

ping - does this really need an issue, or is it ready to go as-is?

Daniel Rosenwasser (@DanielRosenwasser) Ryan Cavanaugh (@RyanCavanaugh) any update?

hm, test failures seem unrelated

Ryan Cavanaugh (@RyanCavanaugh) yay, how soon is "soon"? :-D

Ryan Cavanaugh (@RyanCavanaugh) any update?

Jordan Harband (@ljharb) the reason we ask for an associated issue is that the issue is how we track whether or not we decide we want a PR for something in the first place. I think it's a broadly open question whether calling e.g. Object.keys(42) is a sensical operation, or belongs in the category of Math.max({}, () => {}) and we would like to have a discussion about that first before jumping straight to a code fix.

Ryan Cavanaugh (@RyanCavanaugh) sure, that's fine; there's no harm in closing the PR if you don't decide you want the change. However, I'd argue it's not about "sensical" - it's allowed by the spec, and it was explicitly changed in ES6 because people pass primitives into Object.keys and want to not get an exception.

The primary issue is that the ES5 types are incorrect - they currently allow passing non-object non-nullish primitives, but ES5 disallowed this, and only ES2015 enabled that.

I'd argue it's not about "sensical" - it's allowed by the spec

We've had this discussion on Twitter already 😉

I think this is fine to merge once the failing tests gets fixed.

Tests should now be fixed.

After a longer discussion than expected on the merits of Object.keys(number) vs Object.keys(any[]) vs Object.keys(number | object) we eventually settled on the current overload (object only) being the best fit for the API - basically that Object.keys("hello world") is closer to Math.atan("Valid JS!") than to a "likely intentional" call. We can re-evaluate if we get bug reports from people trying to call it that way on purpose.

I opened this because it’s a bug, because i do it this way on purpose in qs, a very well-used library. I’m disappointed in this outcome.

Ryan Cavanaugh (@RyanCavanaugh) it’s also worth noting - what’s in master does not agree with your decision, because you have the ES5 types as the empty interface. The correction is to make es5 object and es6+ the empty interface. In other words, what you want is what I’m asking for es5 to do, and what you don’t want is what typescript already does.

I think that when this was discussed at the backlog slog, we misunderstood the topic. I'm sorry about that.

The root of the problem is that any sort of change sort of needs to build up its worth, especially when there's some risk of breakage or new conceptual overhead. Regardless of the eventual change, creating the distinction between ES5/ES2015 seems like it would be confusing (i.e. having an overload for object and {}). With the current approach of eventually changing the es5 lib to use object, it would potentially break working existing code in a surprising way.

You already saw the opposite approach - we erroneously assumed the type already was object, since it "felt" like the most reasonable behavior, and making it more permissive seemed questionable.

My intention here was for those targeting es5 to be able to get warning of a runtime exception before it happens. Without this PR, the type system will allow it, but an ES5 runtime will not.

the correction is to make es5 object and es6+ the empty interface.

Without this PR, the type system will allow it, but an ES5 runtime will not.

Jordan Harband (@ljharb) This is what I see in the PR and what we were basing our discussion off of:

I don't see any part of the PR that adds an object-accepting overload or changes the ES5 signature to only accept object. Maybe git didn't merge something properly? I think this is the source of confusion - we were assuming the diff here was a meaningful divergence from the current behavior, but what's in this PR is just the same as what's in the current lib file.

I'm OK taking the PR in its intended form 😉

Thanks, sorry for all the back-and-forth.

PR updated.

Jordan Harband (@ljharb) looks like you need to run gulp baseline-accept after gulp runtests-parallel and commit the result

Ryan Cavanaugh (@RyanCavanaugh) oops, thanks. Done!

This broke VS Code

Ryan Cavanaugh (@RyanCavanaugh) can you elaborate? #31380 (comment) implies it caught a bug in vscode, not broke it.

Jordan Harband (@ljharb) see #31380 (comment)

Ryan Cavanaugh (@RyanCavanaugh) ah, gotcha. that suggests a bug in type narrowing then :-) thanks for the followup.

Let's say you have code like this

declare function keys(obj: object): string[];
function fn<T extends null | object>(arg: T) {
    // Point 1
    if (!arg) return;
    // Point 2
    if (typeof arg !== "object") return;
    // Point 3
    keys(arg);
}

There's an error here because arg is still of type T, not object.

To get the "correct" answer for the type of arg at point 3, you have to combine two separate facts about arg - that it isn't falsy (thus excluding null) and that its typeof is object. The inference system doesn't have a way to track these things right now - simply saying if (foo) { on some foo: T extends ... doesn't change anything, because we don't have a way representation of "the values of T which are truthy`.

We'd need #22348 or its equivalent to track this.

That seems excessively valuable given that truthiness and falsiness are both core and idiomatic mechanisms used for runtime guards. Hopefully that could land soon, and type narrowing improved?

It's not that we don't recognize the value of it, believe me 😉